use string instead of struct backing by dignifiedquire · Pull Request #47 · ipfs/go-cid

dignifiedquire · 2018-05-17T14:44:50Z

We call String all over the place so we should make sure it remains fast.

Stebalien · 2018-05-17T15:42:46Z

I like this. However, it's a massively breaking change so we'll have to be careful (I've received a lot of push-back from @whyrusleeping when trying to do this kind of thing in the past). Also, this makes extracting a multihash from a CID allocate (could be fixed by using a string internally) and means that constructing a CID involves copying bytes. We can fix the former by storing multihashes as strings (multiformats/go-multihash#29) and the latter by adding a function to multihash that writes to a writer (e.g., a string builder) which we'll probably want anyways.

A less breaking-change version would be to use a pointer to a string (or a pointer to, e.g., struct Cid{ string }). However, that will require an additional allocation (although the KeyString method becomes trivial which is my personal goal).

Performance wise, implementing the varint skipping code manually may be faster. That is, to skip two varints, count two bytes with the high-order bit unset.

Regardless, I think we should wait for the libp2p refactor + dht fixes + log fixes + everything else that is currently blocked.

Personally, I'd like to just break things and fix this all at once but this really is a massively breaking change. All cid == nil checks now no longer work.

whyrusleeping · 2018-05-29T06:52:26Z

@dignifiedquire please familiarize yourself with the other attempts to do similar things, primarily: multiformats/go-multiaddr#62

dignifiedquire · 2018-05-29T09:06:09Z

@whyrusleeping I did read through that discussion, but didn't see that there was a final decision made on the direction to go, so this is based on a conversation I had with @Stebalien afterwards.

I am happy to adjust if you and @Stebalien get to an agreement on which version exactly you want to go for in these, just let me know.

kevina · 2018-06-15T20:57:43Z

cid.go

+// - hash mh.Multihash
+type Cid string
+
+var EmptyCid = Cid(string([]byte{}))


Why string([]byte{}) and not just ""?

I would also make this a constant so we have:

const EmptyCid = Cid("")

Also. I find EmptyCid a bit tedious of a name. How out just Nil"? When used outside this package it would be cid.Nil.

kevina

Most of my comments are just suggestions but I am blocking for two reasons (1) The KeyString() method is wrong and needs to be fixed. (2) I opened a second p.r. (#71) where I implemented my suggestions.

kevina · 2018-08-25T06:54:35Z

cid.go

-func (c *Cid) KeyString() string {
-	return string(c.Bytes())
+func (c Cid) KeyString() string {
+	return string(c)


This is wrong. If the version is 0 only the Multihash should be returned.

This should actually be right. If the version is 0, this will only be the multihash.

Ah. Nvm, missed #47 (comment)

kevina · 2018-08-25T06:55:06Z

cid.go

+
+var EmptyCid = Cid(string([]byte{}))
+
+func (c Cid) version() uint64 {


We should export this method, it can be useful.

kevina · 2018-08-25T06:58:15Z

cid.go

-		hash:    mhash,
-	}
+func NewCidV0(mhash mh.Multihash) Cid {
+	return newCid(0, DagProtobuf, mhash)


I find this representation of CidV0 odd. Why not just use the normal binary representation, i.e, just the multihash?

kevina · 2018-08-25T07:01:12Z

cid.go

+// - hash mh.Multihash
+type Cid string
+
+var EmptyCid = Cid(string([]byte{}))


Also. I find EmptyCid a bit tedious of a name. How out just Nil"? When used outside this package it would be cid.Nil.

kevina · 2018-08-25T07:21:25Z

cid.go

-		c.version == o.version &&
-		bytes.Equal(c.hash, o.hash)
+func (c Cid) Equals(o Cid) bool {
+	// TODO: can we use regular string equality?


can we use regular string equality?

Yes.

kevina · 2018-08-25T07:24:25Z

cid.go

+// - version uvarint
+// - codec uvarint
+// - hash mh.Multihash
+type Cid string


Maybe we should make this type Cid struct{ string } to ensure the Cid type is always valid.

kevina · 2018-08-25T07:40:17Z

cid_test.go

+	}
+}
+
+// making sure we don't allocate when returning bytes


But we do now and can't avoid it. I take it BenchmarkBytesV1 is leftover from the first attempt with used []byte as the representation.

Stebalien · 2018-08-25T18:26:40Z

@dignifiedquire for context, this is moving forward due to:

#70, the fact that by-value CIDs will work better with refmt, and the fact that we're reworking a ton of CID stuff anyways for the base32 endeavor.

dignifiedquire · 2018-08-25T18:57:28Z

@Stebalien thanks for the heads up, I was following the other thread. Let me know if you need anything from me, otherwise feel free to take the code and use what you can, and throw away what you don’t like :)

kevina · 2018-08-31T05:17:21Z

Closing in favor of #71

Stebalien and others added 4 commits April 20, 2018 10:30

add String benchmark

93b08a6

We call String all over the place so we should make sure it remains fast.

feat: use CIDs as their byte representation instead of a struct

2639b67

use string instead of []byte as underlying store

760ab82

fix KeyString()

7e417ec

ghost assigned dignifiedquire May 17, 2018

ghost added the status/in-progress In progress label May 17, 2018

dignifiedquire mentioned this pull request May 17, 2018

What about backing cid.Set by a sorted slice rather than a map? #45

Open

kevina reviewed Jun 15, 2018

View reviewed changes

kevina mentioned this pull request Aug 4, 2018

WIP: Make Cid an interface and change default representation to a String #64

Closed

warpfork mentioned this pull request Aug 24, 2018

cid implementation roundup #70

Merged

kevina mentioned this pull request Aug 25, 2018

continuation of use string instead of struct backing #71

Merged

kevina suggested changes Aug 25, 2018

View reviewed changes

kevina mentioned this pull request Aug 29, 2018

Update to use the new string representation of a Multihash. #73

Closed

kevina closed this Aug 31, 2018

ghost removed the status/in-progress In progress label Aug 31, 2018

kevina deleted the feat/bytes branch August 31, 2018 05:17


		var EmptyCid = Cid(string([]byte{}))

		func (c Cid) version() uint64 {

Conversation

dignifiedquire commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stebalien commented May 17, 2018

Uh oh!

whyrusleeping commented May 29, 2018

Uh oh!

dignifiedquire commented May 29, 2018

Uh oh!

kevina Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevina Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevina left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevina Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dignifiedquire commented Aug 25, 2018

Uh oh!

kevina commented Aug 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dignifiedquire commented May 17, 2018 •

edited

Loading

kevina Jun 15, 2018 •

edited

Loading

kevina Aug 25, 2018 •

edited

Loading

kevina left a comment •

edited

Loading

kevina Aug 25, 2018 •

edited

Loading

Stebalien commented Aug 25, 2018 •

edited

Loading